If the command class is always the same, then this tag is probably not useful. I added that in other instrumentation because that can be an important tool for debugging.

Makes sense. Will remove.

What are some examples of command.name()? Does this have the right level of cardinality to be useful?

It would be nice if we could name the resource off of more than just the command name, but perhaps the full command (assuming it doesn't change a lot). Though based on your PR description, it sounds like this isn't possible.

Examples of commands are SET, GET, ZRANK and all the other redis commands described here: https://github.com/xetorthio/jedis/blob/2.9/src/main/java/redis/clients/jedis/Protocol.java#L244
I think even at this granularity it is useful

Having the command name + the arguments to the command instead is something I listed in the PR description as a possible improvement, however there are some things to consider:

The datadog ruby APM does that (SETEX command + arguments):

The NewRelic and Glowroot Java APM for redis only list the Commands like this one. I suspect it has something to do with how the Jedis code is laid out. There are 3 methods:

Method 1:

 protected Connection sendCommand(final Command cmd, final String... args) {
    final byte[][] bargs = new byte[args.length][];
    for (int i = 0; i < args.length; i++) {
      bargs[i] = SafeEncoder.encode(args[i]);
    }
    return sendCommand(cmd, bargs);
  }

Method 2:

protected Connection sendCommand(final Command cmd) {
   // declared above private static final byte[][] EMPTY_ARGS = new byte[0][];
    return sendCommand(cmd, EMPTY_ARGS);
  }

Method 3:

protected Connection sendCommand(final Command cmd, final byte[]... args) {
...
}

As you can see methods 1 and 2 are just wrappers for method 3. In my instrumentation I'm just monitoring method 3 and not converting the byte[] args back to String. Here are some options:

• Convert the byte[] back to String to send the arguments along the command. I decided against this for performance reasons.

• Only monitor Method 1 which has sendCommand(final Command cmd, final String... args). I decided against this because we would lose instrumentation for redis commands that don't have arguments since they use method 2.

• Create separate Advices to monitor method 1 and 2 and not monitor method 3. I tried this but the code ended up convoluted and I didn't get to work. Didn't spend much time trying it tho.

Having this info do you have any suggestions on how to proceed or do you think we should just leave it the way it is (just the command name)?

I am fine having resource name be just the command name. That is something we can iterate on in the future.

Later, if we decide we want to capture the arguments, we can do what the ruby client does and set the full command + arguments in the redis.raw_command tag (ex). The ruby client does have an additional level of quantization, but I don't quite understand it so I don't expect you to implement that.

We just merged #213 which adds a new abstract parent for instrumentation to use. Please rebase from master and change your new instrumentation to instead extends Instrumenter.Configurable, add a constructor that defines some useful names for this instrumentation and also add an override similar to what I did for kafka so that it is disabled by default. This will allow folks to test it for a while if they desire. At a later release we can then remove that and enable it by default.

Nice, will do.

Sorry... one last thing and I think we're good. Generally, the convention is that the version here is the earliest version the instrumentation will work with. Will this work with anything before 2.9?

@tylerbenson I just did a scan on all the tags available in the Jedis Repo (e.g.: https://github.com/xetorthio/jedis/blob/jedis-1.4.0/src/main/java/redis/clients/jedis/Connection.java)

My findings is that the first version that will be supported by this is Jedis 1.4.0 and it will work all the way to 2.10. However the current class in Master has changed the method and it will no longer work in their next release so we should probably cap the support at 2.10

Method in master has a different signature: https://github.com/xetorthio/jedis/blob/master/src/main/java/redis/clients/jedis/Connection.java#L123

Since it hasn't been released yet, our tooling can't really exclude it well. Let's just rename the project to jedis-1.4 and add some classes to identify it differently from pre-1.4. Would you like to do that or would you rather I do it?

@tylerbenson Just want to make sure you are aware that 1.4.0 is a really old version from Nov 15, 2010 and also one of the first versions (https://github.com/xetorthio/jedis/releases?after=jedis-1.5.0-RC1). Before 1.4.0 all the way down to 1.0.0 the method to instrument has this signature: https://github.com/xetorthio/jedis/blob/jedis-1.3.1/src/main/java/redis/clients/jedis/Connection.java#L52

I don't think I will get to it anytime soon so I rather you do it. Thanks.

Also, we will probably have to instrument another class after the next release of Jedis comes out (code in master now) anyways so we should plan for multiple instrumentations either way.

The way we restrict the instrumentation from getting added to a version that isn't supported is by using this kind of thing: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/aws-sdk/src/main/java/datadog/trace/instrumentation/aws/AWSClientInstrumentation.java#L34-L39.

Those classes are verified using a plugin: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/aws-sdk/aws-sdk.gradle#L5-L14

Basically those classes are unique to the versions of the library it supports.

I switched to the protocol instead of the connection since earlier versions of the no-arg sendCommand didn't delegate the same way. Do you see any problems with this @gihad?

The only thing I see is that the sendCommand method from Connection does more work, not sure what's more accurate to instrument in this case. I'm good either way.

@realark I forget... is this needed for spring boot executable jar type scenarios?

It's needed if the class being instrumented is in the method signature of the advice. I think Protocol is being instrumented, so I don't think this is needed.